FED-3254 Officially support Dart 3 #958

greglittlefield-wf · 2024-10-10T18:56:57Z

Related react-dart PR: Workiva/react-dart#409

Motivation

We should support Dart 3.

over_react can currently be consumed in Dart 3, despite not supporting it in its SDK constraints, due to the way Dart 3 treats null-safe packages as compatible.

However, we don't officially support Dart 3, and our automated CI checks don't validate that it works properly in Dart 3.

Also, we still need to support Dart 2 for the time being, so ideally we'd be able to support both Dart 2 and 3 in the same over_react version, and run CI against each Dart version.

Changes

Raise SDK constraints to explicitly support Dart 3
Fix miscellaneous analysis warnings and test failures in Dart 3
- See review comments for specifics on these
Update CI to run against both Dart 2 and Dart 3
- In Dart 3 runs, delete non-null-safe browser test cases, since they don't apply in Dart 3 and won't compile
- Add dart-2-only tag for VM tests that only apply in Dart 2
- Work around test hanging issue in Dart 3, add test timeouts so CI doesn't hang
- Break SBOM job out of matrix, since it can only get run once
Update dev_dependencies:
- Widen build_web_compilers to allow newer versions needed by Dart 3
- Remove unused pedantic and built_value_generator dependencies

Caution

There's a bug in Dart >=3.3.0 <3.5.0 that breaks react-dart behavior in DDC of certain functions passed to components, such as refs and potentially JS callback props: dart-lang/sdk#56897

We can't prevent users on those versions from pulling in react-dart, since they can already resolve to null-safe versions that have already been published.

So, consumers will just have to avoid those versions of Dart, or use dart2js during development or tests.

If needed, we could potentially follow up with a warning that detects this bug and provides a helpful error.

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
  - CI runs in both Dart 2 and 3, and all steps pass
  - Verify the analyzer plugin runs in tools/analyzer_plugin/playground/ in an IDE using Dart 3 (just need to pub get in that directory and you're good)
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Frontend Frameworks Design member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

aviary-wf · 2024-10-10T18:59:15Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

greglittlefield-wf · 2024-10-16T03:15:51Z

test/vm_tests/builder/parsing/members_test.dart

@@ -195,34 +195,31 @@ main() {
          };

          // Loop over the deprecated lifecycle methods
-          legacyLifecycleMethodsMap.keys.forEach((lifecycle) => {


There were some syntax shenanigans going on here, it looks like from a TS/Dart arrow function syntax mixup 🙂.

This set of curly braces and the ones in the below for-loop are actually being interpreted as set literals, not blocks, were causing warnings.

So, I fixed that and switched from forEach to a for-loop while I was at it.

greglittlefield-wf · 2024-10-16T17:03:30Z

lib/src/util/css_value_util.dart

@@ -132,7 +132,7 @@ class CssValue implements Comparable<CssValue> {

  /// Returns whether this value's [number] and [unit] are equal to that of [other].
  @override
-  bool operator ==(dynamic other) {
+  bool operator ==(Object other) {


This fixes a new built-in Dart warning; apparently == will never be passed a null value (== null bypasses operator==).

interesting

greglittlefield-wf · 2024-10-16T17:03:53Z

lib/src/component_declaration/disposable_manager_proxy.dart

@@ -63,21 +63,18 @@ mixin DisposableManagerProxy on react.Component implements DisposableManagerV7 {
      _getDisposableProxy().listenToStream(stream, onData,
          onError: onError, onDone: onDone, cancelOnError: cancelOnError);

-  @override


These weren't actually overrides and were infos in Dart 2, but got upgraded to warnings in Dart 3.

greglittlefield-wf · 2024-10-16T17:07:24Z

...tion/builder_integration_tests/backwards_compatible/null_safe_accessor_integration_test.dart

@@ -96,13 +96,6 @@ void main() {
            testValue: 'test value',
          );
        });
-        test('nullable dynamic with extraneous ? syntax', () {


These test cases tested a dynamic? type as an edge case, which now warns instead of infos when used (because the ? is unnecessary).

I tried suppressing those warnings, but dynamic? also appears in the generated part file, and there's no easy way to suppress that warning for just that file.

Since this was an edge-case that warns when the user authors it, preserving the exact type is covered by other test cases, and we originally added this case "just in case", I opted to just remove this case here and in other files.

greglittlefield-wf · 2024-10-16T17:27:15Z

tools/analyzer_plugin/analysis_options.yaml

@@ -15,8 +15,9 @@ analyzer:
    missing_return: warning
    # Override workiva_analysis_options upgrade to warning
    unused_import: info
-    import_of_legacy_library_into_null_safe: warning


This no longer exists in Dart 3

robbecker-wf · 2024-10-18T18:10:01Z

QA+1 CI passes on all combinations of Dart 2 and analyzer. There are validations in place to check that pub resolves the correct analyzer version.
Verified the plugin worked in IDE using Dart 3
Ran some of the examples using Dart 3

greglittlefield-wf · 2024-10-18T18:18:04Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

Start running CI on Dart 3

4b7627b

rmconsole-wf added the Merge Requirements Unmet label Oct 10, 2024

Fix permissions

b57ea35

greglittlefield-wf added 13 commits October 10, 2024 12:01

Fix formatting check

b9788c2

Fix warnings in Dart 3 (except for in generated files)

36fc646

Remove test cases that produce warnings in Dart 3

78a3004

Remove unused dependencies

c1ad5da

Widen build_web_compilers range to fix DDC compilation on Dart 3

50abd3f

Fix Dart-2-specific import

ff88b62

Exclude non-null-safe builder test cases in Dart 3

d754ed8

Fix test args not being passed

105f01b

Suppress analysis_options include warning in Dart 3

32c5b36

Fix empty string being passed as test arg

9f53995

Remove Dart-2-only lint config causing warning in Dart 3

5412fef

Work around presets ignoring excluded tag args

922d535

Suppress comment references info

3a6933a

greglittlefield-wf force-pushed the dart-3-ci branch from aa16617 to 3a6933a Compare October 10, 2024 21:22

greglittlefield-wf added 9 commits October 10, 2024 14:26

Use Dart stable (main used by mistake)

25622a5

Add timeouts, helpful for when test process hangs at end of run

0331324

Work around test hanging issue

f6719b2

Fix Dart 3 CI trying to run unsound test file

9d1cdd2

Only run SBOM action once

35e8c9c

Dial back matrix to only run on latest Dart 3 version

bfa632a

Fix typo

bcbb6ea

Clean up temporary file

375848e

Officially support Dart 3 in SDK constraint

d64668c

greglittlefield-wf changed the title ~~Start running CI on Dart 3~~ Officially support Dart 3 Oct 16, 2024

Fix error message assertion in Dart 3

8df1dc0

greglittlefield-wf mentioned this pull request Oct 16, 2024

raise package upper bounds and/or migrate to Dart 3 #947

Open

greglittlefield-wf commented Oct 16, 2024

View reviewed changes

This was referenced Oct 16, 2024

FED-3253 Officially support Dart 3 Workiva/react-dart#409

Merged

FED-3255 Allow analyzer 6.x #960

Merged

rmconsole3-wf changed the title ~~Officially support Dart 3~~ FED-3254 Officially support Dart 3 Oct 16, 2024

greglittlefield-wf marked this pull request as ready for review October 16, 2024 23:12

greglittlefield-wf requested review from aaronlademann-wf and robbecker-wf October 16, 2024 23:19

robbecker-wf approved these changes Oct 17, 2024

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Oct 18, 2024

rmconsole-wf added the RM Ready label Oct 18, 2024

rmconsole-wf approved these changes Oct 18, 2024

View reviewed changes

rm-astro-wf merged commit 85ed6a8 into master Oct 18, 2024
11 checks passed

rm-astro-wf deleted the dart-3-ci branch October 18, 2024 18:18

rmconsole-readonly-wk mentioned this pull request Oct 18, 2024

RM-288897 RM-288896 Release over_react 5.4.1 #962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-3254 Officially support Dart 3 #958

FED-3254 Officially support Dart 3 #958

greglittlefield-wf commented Oct 10, 2024 •

edited by robbecker-wf

Loading

aviary-wf commented Oct 10, 2024

greglittlefield-wf Oct 16, 2024

greglittlefield-wf Oct 16, 2024

robbecker-wf Oct 17, 2024

greglittlefield-wf Oct 16, 2024

greglittlefield-wf Oct 16, 2024

greglittlefield-wf Oct 16, 2024

robbecker-wf commented Oct 18, 2024

greglittlefield-wf commented Oct 18, 2024

rmconsole-wf left a comment

FED-3254 Officially support Dart 3 #958

FED-3254 Officially support Dart 3 #958

Conversation

greglittlefield-wf commented Oct 10, 2024 • edited by robbecker-wf Loading

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary-wf commented Oct 10, 2024

Security Insights

Action Items

greglittlefield-wf Oct 16, 2024

Choose a reason for hiding this comment

greglittlefield-wf Oct 16, 2024

Choose a reason for hiding this comment

robbecker-wf Oct 17, 2024

Choose a reason for hiding this comment

greglittlefield-wf Oct 16, 2024

Choose a reason for hiding this comment

greglittlefield-wf Oct 16, 2024

Choose a reason for hiding this comment

greglittlefield-wf Oct 16, 2024

Choose a reason for hiding this comment

robbecker-wf commented Oct 18, 2024

greglittlefield-wf commented Oct 18, 2024

rmconsole-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Oct 10, 2024 •

edited by robbecker-wf

Loading